skip proving the trait goal if possible in NormalizesTo goal
#149533
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
also split the `assembly_and_merge_candidates` for `HostEffect` and `NormalizesTo`
rustbot
added
S-waiting-on-review
adwinwhite
commented
Dec 2, 2025
Contributor
Author
There was a problem hiding this comment.
Not sure whether candidate preference matters to effect goal.
Contributor
There was a problem hiding this comment.
it's whatever :> please add a FIXME(const_traits) to assemble_and_merge_candidates that this was copied from the old projection normalization behavior and might not be what we actually want here
adwinwhite
commented
Dec 2, 2025
| // | ||
| // We currently intentionally do not guide inference this way. | ||
| // Since we skip proving the trait goal in normalizes-to goal now, the normalizes-to | ||
| // goal can successfully resolve the infer var via param env. |
Contributor
Author
There was a problem hiding this comment.
This seems unavoidable since we don't know whether the trait goal will succeed or not at this time.
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
rustbot
added
S-waiting-on-author
lcnr
reviewed
Dec 2, 2025
Contributor
There was a problem hiding this comment.
fun, this isn't quite the behavior of the old trait solver either :3
the old solver only skips trying to prove the trait goal of we've got a builtin trait object impls. I think this ended up not really being necessary after your previous work?
idk... this sucks xd
let's see if the change impacts perf in a major way. If so, we can still avoid assembling impls in some cases at least :3
| D: SolverDelegate<Interner = I>, | ||
| I: Interner, | ||
| { | ||
| pub(super) fn normalize_trait_associated_term( |
Contributor
There was a problem hiding this comment.
Suggested change
| pub(super) fn normalize_trait_associated_term( | |
| pub(super) fn normalize_projection_term( |
that's what we use in AliasTermKind 🤔
| // We currently intentionally do not guide inference this way. | ||
| // Since we skip proving the trait goal in normalizes-to goal now, the normalizes-to | ||
| // goal can successfully resolve the infer var via param env. | ||
| // This causes the stalled ambiguous trait goal to succeed as well. |
Contributor
There was a problem hiding this comment.
this also doesn't compile with the old solver right now 🤔 why?
ah, because the old solver always uses assemble_candidates_from_impls and forces ambiguity unless assembly resulted in a builtin trait object candidate :<
that's wild xx
Comment on lines
+461
to
+463
| // We don't care about overflow. If proving the trait goal overflowed, then | ||
| // it's enough to report an overflow error for that, we don't also have to | ||
| // overflow during normalization. |
Contributor
There was a problem hiding this comment.
Suggested change
| // We don't care about overflow. If proving the trait goal overflowed, then | |
| // it's enough to report an overflow error for that, we don't also have to | |
| // overflow during normalization. | |
| // We don't care about overflow. If proving the trait goal overflowed, then | |
| // it's enough to report an overflow error for that, we don't also have to | |
| // overflow for the const bound. |
or sth
Contributor
There was a problem hiding this comment.
it's whatever :> please add a FIXME(const_traits) to assemble_and_merge_candidates that this was copied from the old projection normalization behavior and might not be what we actually want here
Comment on lines
+106
to
+108
| // If we're normalizing an GAT, we bail if using a where-bound would constrain | ||
| // its generic arguments. | ||
| // FIXME(generic_associated_types): Addresses aggressive inference in #92917. |
Contributor
There was a problem hiding this comment.
flip these two statements. I want the comment to start with the FIXME. Right now it seems like the first line is separate from the content of the FIXME
| &mut self, | ||
| goal: Goal<I, ty::NormalizesTo<I>>, | ||
| ) -> QueryResult<I> { | ||
| let (mut candidates, _) = |
Contributor
There was a problem hiding this comment.
comment: something something we already looked for param_env and alias-bound candidates so we don't have to assemble them again
Contributor
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rust-bors bot
added a commit
that referenced
this pull request
Dec 2, 2025
skip proving the trait goal if possible in `NormalizesTo` goal
rustbot
added
the
S-waiting-on-perf
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Collaborator
|
Finished benchmarking commit (405e127): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary 0.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -21.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 469.029s -> 471.041s (0.43%) |
rustbot
removed
the
S-waiting-on-perf
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
And also split
assembly_and_merge_candidatesforHostEffectandNormalizesTogoals.r? @lcnr